-
Notifications
You must be signed in to change notification settings - Fork 429
Added self signed jwt service account credentials #503
Added self signed jwt service account credentials #503
Conversation
expires_in=self._time_limit) | ||
|
||
@classmethod | ||
def from_service_account_credentials(cls, creds, audience, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@kpayson64 Thanks for contributing this! :) 👍 |
@anthmgoogle @dhermes @nathanielmanistaatgoogle Care to chime in on API and naming conventions here? I'd personally prefer this API (which closely matches pyjwt's): service_account = oauth2client.service_account.ServiceAccount.from_json_keyfile_name('service-account.json')
jwt = oauth2client.jwt.encode(service_account) Additional claims: jwt = oauth2client.jwt.encode(service_account, {'sub': 'user@example.com'}) @nathanielmanistaatgoogle would probably be happy to see less classes. :D |
We could also expose this as |
@jonparrott That misses the mark. These credentials need to be able to be used as a swap-in replacement for credentials. |
@dhermes interesting. Can you expand more on that? JWTs are not credentials, they are signed claims that can be used for authorization. The service account private key is the credential. |
Is the desire to do something like this? credentials = JwtCredentials(service_account_credentials)
credentials.authorize(http)
# http now sends the JWT in the Authorization header If so, I don't see how this is distinct from just making the existing |
For APIs like Cloud Bigtable, the JWT can be used as the access token (so-called scopless credentials). Hence, we want a class to allow this, and maybe have some built-in retry behavior which regenerates the local token (this PR doesn't quite get there either). |
@jonparrott Sorry we posted at essentially the same moment. The difference is that |
Ah, I see, I should get more sleep before discussing auth. So, I think I'd prefer "both". We should separate the logic of generating a JWT vs using the jwt. I suggest we:
This covers all use-cases well and prevents duplicate code. |
@jonparrott @dhermes |
@@ -461,3 +462,56 @@ def create_delegated(self, sub): | |||
result._private_key_pkcs12 = self._private_key_pkcs12 | |||
result._private_key_password = self._private_key_password | |||
return result | |||
|
|||
|
|||
class SelfSignedJWTCredentials(object): |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Okay, after chatting with @jboeuf the API should shake out like this:
This allows for it to satisfy the |
This latest commit has the new API. Also, I have made JWTAccessCredentials inherit GoogleCredentials . I'm holding off on unit tests/documentation until I have an API nailed down. |
|
||
def __init__(self, service_account_creds, additional_claims=None, access_token=None, | ||
token_expiry=None): | ||
super(JWTAccessCredentials, self).__init__(access_token, None, None, None, token_expiry, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
API surface looks good, I completed a round of reviews. Please ping when you're ready for another review. Side note: your file seems to have mixed indentation, please use 4 spaces. |
@jonparrott |
|
||
|
||
def _datetime_to_secs(utc_time): | ||
# time_delta.total_seconds() not supported in Python 2.6 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@jonparrott
If so, is there any expected use case of JWT tokens outside of gRPC I should consider? |
@kpayson64 no, but in general we want to move away from httplib2 (or rather, support urllib3) as soon as possible. If you're writing a net-new project and want thread-safety and proper https, urllib3 is a much more sane way to go. see #128 and gcloud-python#1364. |
I think I really like the API change in the current draft - we're adding one new method to one existing class and that's it for changes to the public surface area of the entire library? Is this the shape this change should have going forward or are there still design pressures pushing this in the direction of "no, a new public class is the right thing for this feature"? |
@jonparrott A side note, AFAICT credentials are not thread safe, and I assumed this was because the underlying httplib2 is not thread safe. Should the new code be thread safe? |
Credentials are thread-safe as long as their storage is thread-safe. On Thu, May 5, 2016 at 11:24 AM kpayson64 notifications@github.com wrote:
|
It seems like we have converged on an API. I think the one unresolved issue is if JWTAccessCredentials should be public. It should be a trivial change to make JWTAccessCredentials public in the future, so I think we should leave it private for now. If we are comfortable with this API, could @jonparrott @nathanielmanistaatgoogle do a final review? |
headers = {} | ||
else: | ||
headers = dict(headers) | ||
headers = self._initialize_headers(headers) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I have addressed the feedback. |
from oauth2client.service_account import ServiceAccountCredentials | ||
return ServiceAccountCredentials.from_json_keyfile_dict( | ||
from oauth2client.service_account import _JWTAccessCredentials | ||
|
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Change content looks good! Please squash commits. Please compose a commit message with at least a paragraph explaining what's going on. Is this a Google-proprietary feature? Is this a Google-specific feature? Is there a publicly available open and unencumbered standard being implemented in this change that you can cite and to which you can link? |
Newer Google APIs can accept JWTs signed using ServiceAccountCredentials for authentication. (See https://jwt.io/). The new behavior for GoogleCredentials.get_application_default() will attempt to use a signed JWT if ServiceAccountCredentials are available and no scope is specified. Upon specifying a scope, OAuth2 authentication will be used.
I've squashed the commits and updated the commit message. |
I'm satisfied; @dhermes, @jonparrott, and @jboeuf: please either also make a final review or explicitly decline to do so? |
I decline 😀 I trust you |
I'm satisfied. On Wed, May 18, 2016, 8:26 AM Danny Hermes notifications@github.com wrote:
|
LGTM. Thanks! Please CC me on the PR that will do the integration with grpc. |
Woohoo! |
@jonparrott |
I probably won't have time to cut a release this week. @nathanielmanistaatgoogle may be able to. |
I'd like to see #506 merged before we release. |
Can you make _JWTAccessCredentials public? Or how else would/show I do something like below. Please also not the hack to set 'aud'. from oauth2client.service_account import _JWTAccessCredentials as JWTAccessCredentials
import httplib2
httplib2.debuglevel = 1
credentials = JWTAccessCredentials.from_json_keyfile_name(
'/path/to/service-account-credentials.json', scopes=['https://www.googleapis.com/auth/datastore'])
# the class method does not pass through he kwargs to __init__()
# this needs to match the service name in the api-config:
credentials._kwargs['aud'] = 'myapi.endpoints.myproject.cloud.goog'
http_auth = credentials.authorize(httplib2.Http(disable_ssl_certificate_validation=True))
resp, content = http_auth.request('https://example.com/v1/myapi', 'GET') Or should people manually do this as e.g. in: |
Initial self signed jwt credential support #252. Here is my thinking with this initial API:
-SelfSignedJWTCredentials class should not be on the OAuth2Client inheritance hierarchy, there are many oauth2 specific methods (like refresh) that don't apply to self signed credentials.
-SelfSignedJWTCredentials can be placed in the service_account module, SelfSignedJWTCredentials are highly coupled with service account credentials, self signed jwts can only be signed using service account credentials
-get_access_token() can be the external interface for generating tokens, it is consistent with the GoogleCredentials API, and there is nothing OAuth2 specific about the naming.
-The fields contained in the JWT are Google specific as far as I can tell, so this should be considered a Google specific class